Support vault provider configs and imported Link wallets - #186
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dad8381. Configure here.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
hiroTamada
left a comment
hiroTamada
left a comment
There was a problem hiding this comment.
approved with two non-blocking suggestions for consolidating secret handling at the validation and response boundaries.
validation: 500 tests passed and bunx tsc --noEmit passed. the edge cases in the inline suggestions were reproduced using the real MCP in-memory transport with mocked upstream responses.
| "Write-only token pair from the same grant. Supply through a trusted backend, never chat. Kernel owns subsequent refresh rotation.", | ||
| ), | ||
| }) | ||
| .strict("Unexpected imported authorization fields."), |
There was a problem hiding this comment.
suggestion (non-blocking): sanitize validation failures at the vault tool boundary rather than maintaining separate safe/unsafe object schemas. .strict("…") changes the message but still includes raw unknown keys in the Zod error. an otherwise valid imported wallet with an extra authorization["review-secret-sentinel"] property returns that sentinel in the MCP error's keys array, before throwVaultError runs.
using ordinary strict schemas behind a sanitized validation boundary would also remove the secretInputObject workaround, whose .passthrough().refine(...) currently advertises additionalProperties: true in tools/list while rejecting extras at runtime. worth covering misplaced secrets at each nesting level with transport tests.
There was a problem hiding this comment.
Addressed in 6e78833. All five vault tools now wrap each input field at registration with a preprocessing boundary that emits only a fixed validation issue on failure. The ordinary strict schemas still drive tools/list and successful callback parsing; secretInputObject and its passthrough workaround are removed. Real MCP transport regressions cover rejected keys at spec/authorization/client/provider_config/tokens depth, rejected discriminator values, config validation, and additionalProperties: false in discovery. Full suite: 517 passed; typecheck and production build passed. Please re-review this commit before merge.
| target: VaultItemTarget, | ||
| secrets: (string | undefined)[] = [], | ||
| ) { | ||
| const projected = redactVaultSecrets( |
There was a problem hiding this comment.
suggestion (non-blocking): apply supplied-secret protection to the complete response, not just the projected item. the hints here are constructed afterward from unsanitized target fields. in a mocked wallet import where a synthetic access token was also accidentally supplied as the wallet key, item.key became [redacted] but both observation hints returned the raw token in arguments.key.
keep the upstream field allowlist, then sanitize the complete response at one output boundary. omit unsafe executable hints rather than replacing their identifiers with unusable redacted values. this would also simplify the current project → stringify → parse → construct more output → stringify sequence.
There was a problem hiding this comment.
Addressed in 6e78833. Public projection is followed by complete-response sanitization with one final serialization, without a stringify/parse roundtrip. Observation and invocation hints containing supplied secrets are omitted, never given redacted executable identifiers. The config tool also sanitizes complete acknowledgments, including delete/404 responses. Transport tests reproduce key/vault/project collisions, unsafe operation names, retained safe hints, and config selector collisions.
2 participants
SDK dependency
Uses the published
@onkernel/sdk0.101.0 (^0.101.0in package.json, pinned in bun.lock), released from commit88e5ccf02ff0b653f8951d38a508a684fe12231b. The stable-SDK merge gate is resolved: the branch now installs, typechecks, and builds using its committed lockfile without a staging dependency or local override.The released vault resource sources match the previously validated preview
ae29b778cecc8aadbf1922c29cd1fcb0464f9ea6. No generated SDK source is committed. Deployment still requires the new API routes and the appropriate vault entitlement; deployed availability was not tested.Summary
manage_vault_provider_configscreate/list/get/update/delete actions for organization-owned Link and AgentCard configs, with organization-scoped write checks, single-page pagination, secret rotation without identity changes, no retries, and existing deletion/error conventions.recovery_required. Existing user-approval, entitlement gating, project scoping, and no-payment-retry behavior remain intact.Validation
Rerun against the published 0.101.0 package after
bun install --force --frozen-lockfile:bun test: 517 passed, 0 failed across 51 files, using mocked transports; covers tool routing, schemas, scopes, pagination, redaction, Kernel-managed compatibility, and recovery behavior.bunx tsc --noEmit: passed.bun run build: passed with dummy build-time OAuth/Clerk configuration; no real authentication or provider operation.git diff --check: passed.Without build-time OAuth configuration, Next.js page-data collection fails on existing required client IDs. The existing
next lintscript is unsupported on Next.js 16; changed-file Prettier was used instead.No live provider calls, production mutations, deployment, merge, or release performed by this PR. Review fixes add generic shared 403 guidance and preserve flat multi-action tool handling of unused fields, with regression coverage. Human approval and latest-commit checks are required before merge.